bpo-9303: Migrate sqlite3 module to _v2 API to enhance performance by palaviv · Pull Request #359 · python/cpython

palaviv · 2017-02-28T20:39:42Z

Use the _v2 API when possible.

…pare_v2

berkerpeksag

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

berkerpeksag · 2017-02-28T21:50:21Z

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:

cpython/Modules/_sqlite/cursor.c

Line 553 in c92e604

if (rc == SQLITE_SCHEMA) {

berkerpeksag · 2017-02-28T22:11:18Z

    Py_BEGIN_ALLOW_THREADS
+#ifdef HAVE_SQLITE3_OPEN_V2
+    rc = sqlite3_open_v2(database, &self->db,
+                         SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL);


Style nitpick: FOO|BAR is more common than FOO | BAR in sqlite3 code base.

berkerpeksag · 2017-02-28T22:14:26Z

    }
    Py_BEGIN_ALLOW_THREADS
+#ifdef HAVE_SQLITE3_OPEN_V2
+    rc = sqlite3_open_v2(database, &self->db,


I think that there is an opportunity to simplify this whole SQLITE_OPEN_URI and HAVE_SQLITE3_OPEN_V2 dance.

(The use of Py_BEGIN_ALLOW_THREADS is a bit weird in line 111 and line 120.)

…re_v2

palaviv · 2017-03-01T19:41:20Z

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

done

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:

cpython/Modules/_sqlite/cursor.c

Line 553 in c92e604

if (rc == SQLITE_SCHEMA) {

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

berkerpeksag

This looks pretty good to me. We just need an entry in Misc/NEWS (please add "Patch by Aviv Palivoda." too)

berkerpeksag · 2017-03-02T11:30:27Z

        (void)sqlite3_reset(st);
    }
+#endif
+


Nitpick: Please remove extra empty line.

berkerpeksag · 2017-03-02T11:32:41Z

    }
    Py_BEGIN_ALLOW_THREADS
+    /* No need to use sqlite3_open_v2 as sqlite3_open(filename, db) is the
+       same as sqlite3_open_v2(filename, db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL)*/


Nitpick: FOO|BAR syntax instead of FOO | BAR.

Also, finish the line with a full stop and space:

/* [...] NULL). */

berkerpeksag · 2017-03-02T12:08:54Z

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

Right, we can still get SQLITE_SCHEMA from sqlite3_step() (in legacy API we need to call sqlite3_reset() to get it), but in this case rc is the return value of sqlite3_reset(). The latter call is not needed when we have the new API since sqlite3_step() calls sqlite3_reset() for us.

palaviv · 2017-03-02T18:52:22Z

I have removed the reset but I left the recompile because we should recompile before calling sqlite3_step again.

berkerpeksag · 2017-03-02T22:17:26Z

    int errorcode;

-    /* SQLite often doesn't report anything useful, unless you reset the statement first */
+#if SQLITE_VERSION_NUMBER >= 3003009


Shouldn't >= be < here?

Good catch!

serhiy-storchaka

Fix an error found by @berkerpeksag.

berkerpeksag

Thanks, Aviv! I will wait for Serhiy's approval and merge this.

Closes #358

Converts 12 BasicBlock-related extern "C" wrapper functions in lir_c_api.cpp to static inline definitions in lir_c_api.h. Each wrapper was a thin reinterpret_cast + field access; layout-pin static_asserts in lir_instr_c_verify.cpp guarantee LirBasicBlock (C) and BasicBlock (C++) field offsets match, making direct field access via cast safe across both languages. Files: M Python/jit/lir/lir_c_api.h +82 / -19 (12 static inline defs added) M Python/jit/lir/lir_c_api.cpp +6 / -68 (12 wrapper bodies removed) Net: -75 / +81 = +6 LOC (header inline definitions slightly larger than collapsed C++ bodies due to per-function comment + cast pattern). Inlined functions (all static inline in lir_c_api.h): jit_lir_block_num_preds -> ((const LirBasicBlock*)b)->num_preds_ jit_lir_block_get_pred -> ((const LirBasicBlock*)b)->predecessors_[i] jit_lir_block_num_succs -> ((const LirBasicBlock*)b)->num_succs_ jit_lir_block_get_succ -> ((const LirBasicBlock*)b)->successors_[i] jit_lir_block_get_last_instr -> ((const LirBasicBlock*)b)->instr_tail_ jit_lir_block_get_first_instr -> ((const LirBasicBlock*)b)->instr_head_ jit_lir_block_num_instrs -> ((const LirBasicBlock*)b)->num_instrs_ jit_lir_block_get_false_succ -> ((const LirBasicBlock*)b)->successors_[1] jit_lir_block_get_section -> (int)((const LirBasicBlock*)b)->section_ jit_lir_block_set_section -> ((LirBasicBlock*)b)->section_ = ... jit_lir_block_get_id -> ((const LirBasicBlock*)b)->id_ jit_lir_block_get_instr_at -> linked-list traversal via instr_head_/next_ Caller audit (preserved by header-inline): cold_block_marker.c: 14 sites dce.c: 4 sites blocksorter.c: 4 sites printer_c.c: 11 sites verify.c: 1 site TOTAL: 34 caller sites, ALL preserved by inline (same function names, same signatures, same return semantics). Behavior preservation: - Field offsets: guaranteed by static_asserts in lir_instr_c_verify.cpp (specifically: id_, successors_, instr_head_, section_ offsets all cross-validated) - num_succs_ / num_preds_ : direct field reads vs C++ predecessors().size() — C++ BlockSpan::size() returns the same num_preds_ field (block.h:24) - get_false_succ : preserves original no-bounds-check semantic (original used successors_[1] without check; matches inline version) - set_section : original called BasicBlock::setSection() which is just section_ = section (block.h:183 confirmed); inline writes field directly - get_instr_at : C inline traverses instr_head_->next_ chain identically to C++ instructions().front() + cur->next_ loop .h-decl <-> .cpp-defn enumeration: COMPREHENSIVE (per c4 lesson). All 12 extern declarations in lir_c_api.h:40-51 mapped to definitions in lir_c_api.cpp:63-129; all 12 inlined; 0 missed; 0 dead methods to delete this round (all 12 have active callers per audit above). .h-inline <-> decl-visibility check: lir_c_api.h is self-contained for the inlined functions — uses LirBasicBlock + LirInstruction (declared in lir_types_c.h, included by lir_c_api.h:15) + JitLirBlock/JitLirInstr typedefs (defined later in same file). LirCodeSection used in set_section is also from lir_types_c.h. No new include needed. PRE-COMMIT BUILD VERIFIED (per supervisor 16:12:20Z compile-before-commit directive, first real test of the workflow per pythia python#359): Build command: cd Python/jit_build/build && cmake --build . --target jit -j 16 (then) cd cpython && make -j16 python Build trailer (verbatim, x86_64, 2026-05-12): [ 24%] Linking CXX static library libjit.a [100%] Built target jit clang++ ... -o python ... -Wl,--start-group Python/jit_build/build/libjit.a ... (link successful, exit 0) Warnings: 5 pre-existing (block-comment in reader_c.h:22, unused-private- field in phoenix_asm_wrapper.h:865 etc.); ZERO new warnings introduced. Binary post-build: stat: 2026-05-12 10:03:36 python sys.version: 3.12.13 (heads/phoenix-asm-integration:6aedfa8d1e, ...) NOTE: version string reports c5 (6aedfa8) because git HEAD at compile-time was c5; binary CONTAINS the c6 inline code (in libjit.a) but the embedded commit-string lags by one commit. Testkeeper Tier 1 rebuild post-c6-commit will produce a binary with c6 commit string. Working tree clean post-build (verified: only the 2 staged c6 files M). Pre-ARM64 APPROVE pending; ABBA waived (inline conversion of byte-equivalent field accessors, no executable-path semantic change beyond removed function- call overhead which the optimizer would have eliminated anyway via LTO).

Add optional use of sqlite3_open_v2, sqlite3_close_v2 and sqlite3_pre…

c92e604

…pare_v2

berkerpeksag reviewed Feb 28, 2017

View reviewed changes

brettcannon added CLA not signed and removed CLA not signed labels Feb 28, 2017

the-knights-who-say-ni added the CLA signed label Feb 28, 2017

berkerpeksag mentioned this pull request Mar 1, 2017

bpo-27645: Supporting native backup facility of SQLite #377

Closed

palaviv added 3 commits March 1, 2017 21:09

Use SQLITE_VERSION_NUMBER

8eccb84

Don't call sqlite3_reset in _pysqlite_seterror in using sqlite3_prepa…

5360795

…re_v2

Revert sqlite3_open change

9185cb9

berkerpeksag reviewed Mar 2, 2017

View reviewed changes

palaviv added 3 commits March 2, 2017 20:44

Don't reset the statment if using sqlite3_prepare_v2

f7dcd02

CR fixes

d4d9c4e

Add NEWS entry

64c3bb7

Merge remote-tracking branch 'upstream/master' into sqlite-v2-api

645d959

serhiy-storchaka approved these changes Mar 2, 2017

View reviewed changes

berkerpeksag reviewed Mar 2, 2017

View reviewed changes

serhiy-storchaka requested changes Mar 3, 2017

View reviewed changes

palaviv added 2 commits March 3, 2017 10:11

Fix ifdef in _pysqlite_seterror

f1ddaba

Merge remote-tracking branch 'upstream/master' into sqlite-v2-api

3157f4c

berkerpeksag approved these changes Mar 3, 2017

View reviewed changes

serhiy-storchaka approved these changes Mar 3, 2017

View reviewed changes

berkerpeksag merged commit 86a6705 into python:master Mar 3, 2017

palaviv mentioned this pull request Mar 4, 2017

bpo-26187: Tests for sqlite3 trace callback #434

Closed

coleifer mentioned this pull request Jul 22, 2019

Is sweepea using sqlite3_close_v2 ? coleifer/sweepea#1

Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Use Python 3.7.8 (#359)

5f46257

Closes #358

Uh oh!

Conversation

palaviv commented Feb 28, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palaviv commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Mar 2, 2017

Uh oh!

palaviv commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

berkerpeksag Feb 28, 2017 •

edited

Loading

palaviv commented Mar 1, 2017 •

edited

Loading